Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

firstParty/native flag for elixir/erlang/go #4916

Merged

Conversation

haidong
Copy link
Contributor

@haidong haidong commented Jul 30, 2024

To address #4795, this is part of a series of PRs that add isFirstParty or isNative flags to instrumentation registries that lack them.

This batch touches Elixir, Erlang, and Go instrumentation registries.

There are a number of repos that belong to individual maintainer as opposed to a corp/foundation entity. Please point out any errors and I will correct.

Oh, about where I placed the line: if there is a package: line, then the flag is placed right above it. Otherwise, I placed it under the createdAt: line, which makes it usually the last non empty line of the file. I did it this way before I observed this pattern in other files before modifications.

Thanks!

@haidong haidong requested a review from a team July 30, 2024 20:01
Copy link
Member

@svrnm svrnm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for taking a look into this, note that there are many you marked as first party that aren't. I tagged some of them but not all.

A first party instrumention is given if and only if the instrumented library and the instrumentation library are created by the same party

A native instrumentation is given if and only if the instrumented library does not need an external instrumentation library and comes with otel out of the box.

data/registry/instrumentation-go-xsam-database-sql.yml Outdated Show resolved Hide resolved
data/registry/instrumentation-go-zap.yml Outdated Show resolved Hide resolved
data/registry/instrumentation-go-uptrace-otelsql.yml Outdated Show resolved Hide resolved
data/registry/instrumentation-go-sqs.yml Outdated Show resolved Hide resolved
data/registry/instrumentation-go-splunksqlx.yml Outdated Show resolved Hide resolved
data/registry/instrumentation-go-splunkgraphql.yml Outdated Show resolved Hide resolved
data/registry/instrumentation-go-splunkgorm.yml Outdated Show resolved Hide resolved
data/registry/instrumentation-go-splunkelastic.yml Outdated Show resolved Hide resolved
data/registry/instrumentation-go-splunkdns.yml Outdated Show resolved Hide resolved
data/registry/instrumentation-go-splunkclient-go.yml Outdated Show resolved Hide resolved
@@ -14,3 +14,4 @@ authors:
urls:
repo: https://github.com/dubonzi/otelresty
createdAt: 2023-07-13
isFirstParty: true
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
isFirstParty: true
isFirstParty: false

Comment on lines 15 to 17
repo: https://go.opentelemetry.io/contrib/instrumentation/github.com/gocql/gocql
createdAt: 2022-12-07
isFirstParty: true
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that repo URL is incorrect, but also I don't think this is first party

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What would be the correct URL? And how do you find it?

Comment on lines 16 to 18
repo: https://go.opentelemetry.io/contrib/instrumentation/github.com/bradfitz/gomemcache
createdAt: 2022-12-07
isFirstParty: true
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that repo URL is incorrect, but also I don't think this is first party

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What would be the correct URL and how do you find it?

@svrnm
Copy link
Member

svrnm commented Aug 9, 2024

@haidong please take a look

@haidong
Copy link
Contributor Author

haidong commented Aug 11, 2024

@haidong please take a look

working on it now. Should be able to resolve your questions today.

@haidong
Copy link
Contributor Author

haidong commented Sep 9, 2024

Thank you, @svrnm !

I accepted your suggestions one by one first in the Conversation tab. I then found I could do it in bulk at Files Changes tab, which is what I did to the rest of your suggestions.

Please take a look again.

@haidong haidong requested a review from svrnm September 10, 2024 13:29
@svrnm
Copy link
Member

svrnm commented Sep 11, 2024

/fix:all

@opentelemetrybot
Copy link
Collaborator

@opentelemetrybot
Copy link
Collaborator

fix:all was successful.

IMPORTANT: (RE-)RUN /fix:all to ensure that there are no remaining check issues.

@svrnm svrnm added this pull request to the merge queue Sep 11, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to a conflict with the base branch Sep 11, 2024
@haidong
Copy link
Contributor Author

haidong commented Sep 13, 2024

Please let me know if I can help in resolving the remaining conflicts. Thanks!

@svrnm
Copy link
Member

svrnm commented Sep 13, 2024

Please let me know if I can help in resolving the remaining conflicts. Thanks!

those files have been deleted, since Go SIG is no longer maintaining them, so to resolve the conflict work on your PR locally, merge from main and make sure those fails stay removed.

To address open-telemetry#4795, this is part of a series of PRs that add isFirstParty
or isNative flags to instrumentation registries that lack them.

This batch touches Elixir, Erlang, and Go instrumentation registries.
@haidong
Copy link
Contributor Author

haidong commented Sep 13, 2024

those files have been deleted, since Go SIG is no longer maintaining them, so to resolve the conflict work on your PR locally, merge from main and make sure those fails stay removed.

Done. Please review when convenient. Thanks @svrnm

@haidong
Copy link
Contributor Author

haidong commented Sep 14, 2024

I noticed the Links / REFCACHE updates error. Following the suggestion, I ran npm run test-and-fix but it failed with the following:

...
> _build
> npm run _hugo -- -e dev --buildDrafts --baseURL "${DEPLOY_PRIME_URL:-http://localhost}"


> _hugo
> hugo --cleanDestinationDir -e dev --buildDrafts --baseURL http://localhost

Error: Unable to locate config file or config directory. Perhaps you need to create a new site.
       Run `hugo help new` for details.

Total in 0 ms

Help please, thanks!

PS. After reading https://opentelemetry.io/docs/contributing/pr-checks/, I ran npm run check:links with the same results.

@haidong
Copy link
Contributor Author

haidong commented Sep 14, 2024

/fix:all

@opentelemetrybot
Copy link
Collaborator

@opentelemetrybot
Copy link
Collaborator

fix:all was successful.

IMPORTANT: (RE-)RUN /fix:all to ensure that there are no remaining check issues.

@haidong
Copy link
Contributor Author

haidong commented Sep 15, 2024

/fix:all

@opentelemetrybot
Copy link
Collaborator

@opentelemetrybot
Copy link
Collaborator

fix:all was successful.

IMPORTANT: (RE-)RUN /fix:all to ensure that there are no remaining check issues.

@svrnm svrnm added this pull request to the merge queue Sep 16, 2024
Merged via the queue into open-telemetry:main with commit b93ec7f Sep 16, 2024
5 checks passed
@haidong
Copy link
Contributor Author

haidong commented Sep 17, 2024

Thanks so much @svrnm for your feedback and patience.

If you have no objections, I'll proceed with the rest instrumentations. I'll once again do it in batches, a few programming languages at a time, so the number of files touched won't be overwhelming.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants